update: silent failure on non-matching package specs with --breaking #16276
Conversation
When using 'cargo update -Zunstable-options --breaking <spec>', package specifications that don't match any dependency in the workspace or lockfile are silently ignored instead of reporting an error. This leads to confusing behavior where users think they're updating a package that doesn't exist. Fix this by tracking which specs match dependencies during the upgrade process. After processing all workspace members, verify that each requested spec matched at least one direct dependency or exists in the lockfile as a transitive dependency. Report an error for any spec that matches neither. The fix preserves existing behavior for renamed dependencies and non-registry sources while ensuring proper error reporting for genuinely missing packages. Signed-off-by: Charalampos Mitrodimas <[email protected]>
rustbot
added
Command-update
S-waiting-on-review
| #[cargo_test] | ||
| fn update_breaking_missing_package_error() { |
There was a problem hiding this comment.
Something our contrib docs encourage is adding tests in the commit before with them passing, showing the old behavior. Then in this commit the test then gets updated to show the new behavior and the diff between them shows how the behavior changed. This does a good job communicating what a PR is doing (and tests the tests)
| // Spec didn't match any direct dependencies | ||
| // Check if it matches any package in the lockfile (including transitive deps) | ||
| let matches_lockfile = if let Some(ref resolve) = previous_resolve { | ||
| spec.query(resolve.iter()).is_ok() | ||
| } else { | ||
| false | ||
| }; | ||
|
|
||
| if !matches_lockfile { |
There was a problem hiding this comment.
Why are we skipping these? They look to be unused otherwise If we do error, we could leverage the logic you wrote to help inform the user that it does exist but that it isn't selectable
| for spec in &to_update { | ||
| if !matched_specs.contains(spec) { |
There was a problem hiding this comment.
What if we report these all at once rather than just the first?
| let name = dependency.package_name(); | ||
| let renamed_to = dependency.name_in_toml(); | ||
|
|
||
| if !to_update.is_empty() { |
There was a problem hiding this comment.
why is this check present? If its empty, the loop just won't iterate
| if !to_update.is_empty() { | ||
| // Check if this dependency should be upgraded based on the specs | ||
| let should_upgrade = to_update.iter().any(|spec| { |
| // Check if any spec matches this dependency by name | ||
| for spec in to_update.iter() { | ||
| if spec.name() == name.as_str() { | ||
| // Mark this spec as matched (exists in workspace) | ||
| matched_specs.insert(spec.clone()); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Why is this here and not at the very bottom of the function when we actually make the change? This ends up duplicating the to_update check and considers it matched even when we won't upgrade it.
| // Check if any specs were not matched against direct dependencies | ||
| if !to_update.is_empty() { | ||
| // Load the lockfile to check for transitive dependencies | ||
| let previous_resolve = ops::load_pkg_lockfile(ws)?; | ||
|
|
||
| for spec in &to_update { | ||
| if !matched_specs.contains(spec) { |
There was a problem hiding this comment.
Seems like this could be made simpler by replacing matched_specs with remaining_specs = to_update.clone(); (maybe switched to an IndexSet) and removing entries as we process them.
3 participants
When using 'cargo update -Zunstable-options --breaking ', package specifications that don't match any dependency in the workspace or lockfile are silently ignored instead of reporting an error. This leads to confusing behavior where users think they're updating a package that doesn't exist.
Fix this by tracking which specs match dependencies during the upgrade process. After processing all workspace members, verify that each requested spec matched at least one direct dependency or exists in the lockfile as a transitive dependency. Report an error for any spec that matches neither.
The fix preserves existing behavior for renamed dependencies and non-registry sources while ensuring proper error reporting for genuinely missing packages.
Closes #16258